Skip to content

feat(permissions): add a command to reconcile the groups with their f… - #268

Merged
mateodurante merged 2 commits into
developfrom
feature/syncgroups
Aug 6, 2026
Merged

feat(permissions): add a command to reconcile the groups with their f…#268
mateodurante merged 2 commits into
developfrom
feature/syncgroups

Conversation

@mateodurante

Copy link
Copy Markdown
Contributor

This pull request introduces a new Django management command, syncgroups, to reconcile group permissions with those defined in a fixture, and adds comprehensive tests for its behavior. The command ensures group permissions are additive (never removing admin-added permissions unless explicitly requested), supports dry-run and check modes, and reports on potentially unusable permissions. The accompanying test suite verifies all key scenarios and options.

New management command:

  • Added ngen/management/commands/syncgroups.py, implementing a syncgroups command that:
    • Reads group/permission definitions from a JSON fixture (defaulting to ngen/fixtures/group.json).
    • Grants missing permissions to groups, optionally prunes extra permissions, and can create missing groups.
    • Supports --dry-run, --check, --group, and --prune options for flexible operation.
    • Reports when groups have write permissions without the necessary view permissions for API usability.

Testing:

  • Added ngen/tests/commands/test_syncgroups.py with a comprehensive test suite covering:
    • Granting missing permissions, preserving admin-added permissions, pruning, dry-run, and check modes.
    • Creation of new groups, limiting reconciliation to specific groups, error handling for unknown groups, and reporting on unusable permissions.

…ixture

The permissions of the roles live in ngen/fixtures/group.json, but that
fixture is only loaded on a brand new installation: loaddatafirsttime skips
everything when the user table is not empty. So when a release adds a
permission to a role, a new model or a view_ that was missing, the
installations already running never get it.

The two ways out were both bad. 'loaddata group' replaces the permissions of
every group with the ones of the fixture, so it silently drops whatever an
administrator granted or revoked. A data migration would rewrite decisions
that belong to the operator, unattended, on upgrade.

syncgroups is explicit and additive: it grants the permissions of the fixture
the group does not have, creates the groups the fixture defines and the
installation does not, and touches nothing else. --prune opts into removing
what the fixture does not define, --dry-run reports without applying,
--check exits with an error if there is anything pending, for CI, and
--group limits it to one role.

It also reports the roles holding add, change or delete on a model without
its view. DRF maps GET to view_<model>, so those permissions cannot be used
through the api at all: it is how the Incident Responder role ended up unable
to read the playbooks it is meant to run.
Copilot AI lite review requested due to automatic review settings August 4, 2026 12:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new Django management command, syncgroups, to reconcile auth.Group permissions against the shipped fixture, and introduces a dedicated test suite to verify core behaviors (additive sync, prune, dry-run/check, and reporting).

Changes:

  • Added ngen/management/commands/syncgroups.py implementing fixture-based group/permission reconciliation with --dry-run, --check, --group, and --prune.
  • Added ngen/tests/commands/test_syncgroups.py covering the command’s key scenarios and options.
  • Added ngen/tests/commands/__init__.py to ensure the new test module is packaged/discoverable.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
ngen/management/commands/syncgroups.py Implements the syncgroups command: fixture parsing, permission reconciliation, and usability reporting.
ngen/tests/commands/test_syncgroups.py Adds a comprehensive test suite for syncgroups behaviors and CLI options.
ngen/tests/commands/init.py Initializes the commands test package.
Suppressed comments (1)

ngen/management/commands/syncgroups.py:167

  • When get_or_create() creates a missing group, sync_group() prints “+ created” but the returned change count doesn’t include the creation. This can cause --check to miss a required change for groups that should exist but have no permission diffs.
        return len(to_add) + len(to_remove)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ngen/management/commands/syncgroups.py Outdated
Comment thread ngen/management/commands/syncgroups.py
Comment thread ngen/management/commands/syncgroups.py Outdated
Review of the branch:

- Creating a group counts as a change. It was only counting the permissions
  it granted, so a group the fixture defines with none would be created while
  the command reported everything in sync, and --check would pass.
- The report of write permissions without their view is skipped on a dry run
  with pending changes: it would list the very permissions that applying
  those changes may make usable.
- The permissions of the fixture are resolved against a single query instead
  of one per entry. The shipped fixture holds 544 of them, so the command
  went from 546 queries to 17.
@mateodurante
mateodurante merged commit d4ff819 into develop Aug 6, 2026
6 checks passed
@mateodurante
mateodurante deleted the feature/syncgroups branch August 6, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants